-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Defer types like keyof (T & {})
#49696
Conversation
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at ab04c57. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at ab04c57. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at ab04c57. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at ab04c57. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - main..49696
System
Hosts
Scenarios
Developer Information: |
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@ahejlsberg |
type K04 = keyof void; // never | ||
type K05 = keyof undefined; // never | ||
type K06 = keyof null; // never | ||
type K05 = keyof undefined; // string | number | symbol | ||
type K06 = keyof null; // string | number | symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, as a user - this doesn't quite make any sense. It's super hard for me to reason about what kind of runtime behavior this is supposed to represent. I've also checked it with this PR and this creates an unsoundness and might lead to runtime errors like here:
function test<T, K extends keyof T>(t: T, k: K) {
return t[k]; // oops, runtime error if we allow the call below
}
test(null, 'foo') // OK with this patch, error with 4.7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the new 4.8 behavior, test
itself should have an error in its implementation due to not having a { }
constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First experiment wasn't optimal, now deferring types like keyof (T & {})
such that effects of removing undefined
and null
are properly reflected.
keyof undefined
and keyof null
same as keyof never
keyof (T & {})
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at b457868. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at b457868. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at b457868. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at b457868. You can monitor the build here. |
@ahejlsberg |
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@ahejlsberg Here they are:Comparison Report - main..49696
System
Hosts
Scenarios
Developer Information: |
I've noticed that this PR didn't introduce the reported issue from #49681 as a test case. Is that alright? Or perhaps just an accidental omission? |
// We allow an unconstrained object of a generic type `T` to be indexed by a key of type `keyof T` | ||
// without a check that the object is non-undefined and non-null. This is safe because `keyof T` | ||
// is `never` (meaning no possible keys) for any `T` that includes `undefined` or `null`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this sort of an unusual thing to "defer" the error reporting to the call site? I'm not totally sure what I think about this yet but my first thought was that it's a special case in the algorithm that might be hard to explain to people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this build on top of the fact that T[never]
is always OK and never
is always assignable to everything so perhaps this fits the existing rules just OK. I'll take consolation in the fact @RyanCavanaugh's first instinct was that those should raise errors too 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the test and the comment to capture the behavior for future reference. It's nothing new actually. We have the same behavior in 4.7 with the old NonNullable<T>
based on a conditional type. This PR ensures we carry it forward in 4.8 with NonNullable<T>
based on T & {}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - but I'm not thinking here from the PoV of NonNullable
at all. My confusion/concern was solely about unconstrained generics and your changes from #49119, I've assumed that given those changes this indexed access would error nowadays. Since the base constraint of unconstrained T
is unknown
it feels off that we can use indexed access on it without refining the type somehow first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may seem surprising at first, but it is actually quite meaningful because we can permit t[k]
without requiring local proof that t
is non-undefined and non-null. Such proof would add unnecessary overhead for instantiations where the type argument for T
is already known not to be undefined or null.
Ordinarily, types of the form
keyof (A & B)
are normalized tokeyof A | keyof B
. However, for types likekeyof (T & {})
, this becomeskeyof T | keyof {}
, which becomes justkeyof T
, losing the fact thatundefined
andnull
have been removed fromT
. With this PR we defer the normalization for intersection types that include{}
.Fixes #49681.